ct: Include actual error reason in CTH failure test log - #11231

• potentially incompatible ?
• deliver to maint-29 only?

11.4 AP-4: Failure term shape change

• WHY: the CTH failure reason changes from Str to
  {Str, Stacktrace}. Where it shows up, per the updated
  ct_hooks_SUITE expectations:

  • tc_done: {Suite, TC, {failed, {error, {Str, Trace}}}}
  • tc_auto_skip for later cases:
    {Suite, TC, {failed, {Suite, init_per_suite, {failed, {Str, Trace}}}}}
  • on_tc_fail/4 and on_tc_skip/4 hook callbacks receive the same
    terms.

  Custom event handlers or hooks (CI integrations, rebar3 plugins,
  internal tooling) that match on a string will now see a tuple. In
  OTP itself, only ct_hooks_SUITE matched on "CTH call failed"
  (checked with git grep on maint). Separately, any test case
  failing with a printable-string reason now logs it without quotes.
  Stack frames for undef and function_clause include call
  arguments (for a hook, possibly the whole Config), and these now
  travel in events as well.

• Classification: potentially incompatible (Fix makefile in for xref mod app escript #8). The new term
  stays within the documented FailReason contract
  ({error,{RunTimeError,StackTrace}}), but consumers that relied on
  undocumented details see a different term or text.

• Precedent: common_test has shipped changes like this on maint
  with the *** POTENTIAL INCOMPATIBILITY *** tag
  (lib/common_test/doc/notes.md on maint):

  • Common_Test 1.17.2 and 1.15.4.2 (both patch releases):
    post_init_per_testcase hook failures handled uniformly however
    the hook process failed. This is the closest analogue: a
    hook-failure behaviour change.
  • OTP-12279: tc_done Result changed to ok to match the docs.

• WHAT: no need to move the PR to master. The release note
  should carry the tag. The earlier "maint vs master" question and
  the "keep it a string" alternative are dropped: a string-only
  design would also break exact-string matchers, since the text
  changes too.

• HOW: OTP-20192 release note (Monitor OTP), for example:
  "When a CT hook callback fails, the failure reason now includes the
  exception class and reason, and is reported as
  {ReasonString, Stacktrace} instead of a plain string.
  *** POTENTIAL INCOMPATIBILITY ***"

AP-5 (Blocking, team rule #30): tab characters in 2 new or
modified lines:

• test_server_ctrl.erl (unknown-location progress clause, about
  line 4162):
  \t [escape_chars(io_lib:format(FStr, [FormattedReason]))]),
• ct_hooks_SUITE.erl (faulty_cth_long_reason/1, line 162):
  \t [crash_post_end_tc_cth],Config).

The surrounding code uses tabs, but the team rule requires spaces in
new and modified lines. CI will also reject them (verified): the
check-files job (.github/workflows/reusable-check-files.yaml on
maint, step "Detect whitespace errors") runs
git diff --check --merge-base with core.whitespace=tab-in-indent.
Running the same command locally against upstream/maint:

lib/common_test/src/test_server_ctrl.erl:4162: tab in indent.
lib/common_test/test/ct_hooks_SUITE.erl:162: tab in indent.
→ exit 2

This hasn't shown up yet only because CI hasn't run since the tabs were
added (#4.1).

update commit msg pls

5.3 AP-6: Empty failure reason for error([])

Problem. With the PR, a test case that fails with the reason []
(for example erlang:error([]) or exit([])) gets an empty
=== Reason: line. Before the PR, the line showed [].

Why it happens. The new PF fun in
test_server_ctrl:do_format_exception/2 (PR head line 4424) prints
the error with ~ts (raw text, no quotes) whenever it is a printable
string. io_lib:printable_unicode_list([]) returns true, so [] is
treated as an empty string and printed as nothing.

Action (for Mikaka27). Add Str =/= [] to the guard of the first
PF clause, so [] falls through to the normal term printing:

PF = fun(Str, I) when is_list(Str), Str =/= [], Str =:= Error ->

That is the only change AP-6 asks for. Optionally, add a unit test
for do_format_exception/2 with [] (see AP-8).

Impact. Minor. Failing with [] is unusual, but when it happens
the log gives no hint about what went wrong.

Evidence: other inputs checked. I ran a copy of the PR's PF
logic with the fresh bin/escript. The PF fun formats the reason
of every failing test case, not only CTH failures, so I tried
several inputs:

Note on ~tkp (AP-1, not part of AP-6). The PF fun uses ~tp,
which on maint-28 and later should be ~tkp (map keys sorted, from
f35c2a9d2b). Until now this was handled in the opu forward merges.
After the planned rebase onto maint-29 (AP-2), it has to be in the PR
itself: both PF clauses should use tkp. The k modifier exists
since OTP 27, so this is not a technical limitation.

what do you think about simple verification? can you resist 2nd time ;-)?

6. AP-8: Tests Don't Check the Log Output

Problem. The PR's feature is the === Reason: line in the test
case log, but no test reads that log. The tests only check event
payloads (tc_done, on_tc_fail), and those are built in
ct_hooks / ct_framework before test_server_ctrl formats
anything.

Evidence (#5.4). With all the PR's test_server_ctrl.erl changes
removed (shift_stacktrace/2, the new PF, the prefix handling), the
4 cases still pass, while the log regresses: quoted string, literal
\n, and the doubled === === Reason: prefix. So a later change can
break the feature, or bring back the doubled-prefix bug, without any
test failing.

Action (for Mikaka27). Extend faulty_cth_undef (or
faulty_cth_long_reason) with a check on the rendered test case log.
Optional: AP-6 coverage, see below.

Is it complex? No: about 20 lines, in the test suite only, verified to work.
Everything it needs already exists in ct_hooks_SUITE:

• do_test/4 (line 484) already returns the start options, which
  contain logdir.
• cth_log_formatter/1 (line 315) and verify_cth_log_output/3
  (line 535) already do the same thing: filelib:wildcard/1 for the
  test case log under logdir, file:read_file/1, then match lines.

Tested sketch (4-space indentation). I applied it locally to the PR
head, then reverted it:

• with the PR: faulty_cth_undef passes
• with the base test_server_ctrl.erl (as in Yecc better error message #5.4): it fails
  with {badmatch,false}

So it detects exactly the regression the current tests miss.

faulty_cth_undef(Config) when is_list(Config) ->
    StartOpts = do_test(faulty_cth_undef, "ct_cth_empty_SUITE.erl",
                        [undef_cth], Config),
    Logdir = proplists:get_value(logdir, StartOpts),
    %% ct_test_support runs the inner test twice, so expect >= 1 log
    TCLogs = filelib:wildcard(
               filename:join(Logdir,
                             "ct_run*/cth.tests*/run*/"
                             "ct_cth_empty_suite.init_per_suite.html")),
    [_|_] = TCLogs,
    lists:foreach(
      fun(TCLog) ->
              {ok, Bin} = file:read_file(TCLog),
              Log = binary_to_list(Bin),
              %% reason line with class and reason
              true = nomatch =/=
                  string:find(Log, "=== Reason: undef_cth:pre_init_per_suite/3 "
                                   "CTH call failed: error:undef"),
              %% stack frames follow the reason
              true = nomatch =/= string:find(Log, "in function  lists:flaten/1"),
              %% no doubled prefix
              nomatch = string:find(Log, "=== === Reason:")
      end, TCLogs).

The one surprise while testing it: ct_test_support runs every inner
test twice, so there are 2 matching logs. A [TCLog] = … match
fails with badmatch; loop over all logs, as cth_log_formatter/1
does.

Effort: about 30 minutes, including a test run (each ct_hooks_SUITE case takes ~5 s here). Change size: +22 / −2 lines in ct_hooks_SUITE.erl, nothing in src.

Possible pitfalls, low risk:

• The HTML escapes <, > and & (escape_chars/1). The strings
  above contain none of them.
• Don't assert the exact indentation of faulty_cth_long_reason;
  it depends on the module name length and line width (see the next
  paragraph).

Optional: AP-6 coverage. A direct unit test of
do_format_exception/2 for [], [0,1,2] and a multi-line string is
not cheap: the function isn't exported, and test_server_ctrl has
no -ifdef(TEST) export. It would need a new export or a test helper
in src. Instead, add a small test case to an existing data suite that
fails with erlang:error([]) and check its === Reason: line with
the same pattern as above. That's another ~15 lines.

Side note on the existing test. test_events(faulty_cth_long_reason)
hard-codes column widths (Ind(69), Ind(70) …). These come from the
~tp in ct_hooks (verified in #5.4), not from shift_stacktrace/2,
so the test is fragile against a rename of the hook module or a
change to the default line width, while still not testing
test_server_ctrl. Not a blocker; worth a comment in the test.

On the earlier dismissal. Mikaka27 dismissed Copilot's version of
this point as a "long standing problem, output of common test is not
tested". That's true in general, but in this PR the log output is
the feature (ERIERL-1336), and the infrastructure to test it is
already in the same suite, so the general argument is weaker here.